[PM-33500] - delete attachments from deleted ciphers#7208
[PM-33500] - delete attachments from deleted ciphers#7208jaasen-livefront wants to merge 5 commits intomainfrom
Conversation
|
New Issues (2)Checkmarx found the following issues in this Pull Request
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7208 +/- ##
==========================================
+ Coverage 57.68% 57.69% +0.01%
==========================================
Files 2035 2035
Lines 89645 89663 +18
Branches 7993 7997 +4
==========================================
+ Hits 51709 51733 +24
+ Misses 36072 36065 -7
- Partials 1864 1865 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Claude finished @jaasen-livefront's task in 2m 26s —— View job Code Review: PR #7208 — Delete attachments from deleted ciphers
Result: Clean — No Issues FoundOverall Assessment: APPROVE This PR fixes orphaned attachment files left behind when ciphers are bulk-deleted, an organization vault is purged, or an organization is deleted. The latest revision (addressing prior reviewer feedback) cleanly centralizes attachment cleanup into a new
This eliminates the repeated for-loops that earlier reviewers flagged and keeps Test coverage is thorough with updated assertions for the org delete path and a new dedicated test ( |
gbubemismith
left a comment
gbubemismith
left a comment
There was a problem hiding this comment.
Added a few suggestions
| } | ||
|
|
||
| // Fetch cipher IDs before DB deletion so we can clean up attachment storage | ||
| var orgCiphers = await _cipherRepository.GetManyByOrganizationIdAsync(organization.Id); |
There was a problem hiding this comment.
I think we can improve this a bit. We can just filter and get ciphers with attachments and only return the ciphers Ids
| var orgCiphers = await _cipherRepository.GetManyByOrganizationIdAsync(organization.Id); | |
| var cipherIdsWithAttachments = | |
| (await _cipherRepository.GetManyByOrganizationIdAsync(organization.Id)) | |
| .Where(c => c.GetAttachments()?.Count > 0) | |
| .Select(c => c.Id); |
| } | ||
|
|
||
| // Fetch cipher IDs before DB deletion so we can clean up attachment storage | ||
| var orgCiphers = await _cipherRepository.GetManyByOrganizationIdAsync(organizationId); |
There was a problem hiding this comment.
We can do same filtering here and only get ciphers with attachments and select the cipher ids.
|
|
||
| foreach (var cipher in orgCiphers) | ||
| { | ||
| await _attachmentStorageService.DeleteAttachmentsForCipherAsync(cipher.Id); |
There was a problem hiding this comment.
This is fine, but I think it'd be nicer if we followed the format above and added a method such like DeleteAttachmentsForOrganization then supply only the organization ID like we do for the above applicationCacheService.
The for loop can then live in that method and this file becomes a bit cleaner
| // Clean up attachment files from storage | ||
| foreach (var cipher in deletingCiphers) | ||
| { | ||
| await _attachmentStorageService.DeleteAttachmentsForCipherAsync(cipher.Id); |
There was a problem hiding this comment.
By doing what I suggested above, we can probably also get rid of another for loop here as well
|
|
||
| foreach (var cipher in orgCiphers) | ||
| { | ||
| await _attachmentStorageService.DeleteAttachmentsForCipherAsync(cipher.Id); |
There was a problem hiding this comment.
By having 1 spot where we only supply the organization ID to pass, we can get rid of all these for loops! 😄
JaredScar
left a comment
JaredScar
left a comment
There was a problem hiding this comment.
Good stuff! Just a few small cleanups and should be good to go
|
@gbubemismith @JaredScar I've addressed all the feedback in 834a1b9. Thanks! |
| var ciphersWithAttachments = (await _cipherRepository.GetManyByOrganizationIdAsync(organizationId)) | ||
| .Where(c => c.GetAttachments()?.Count > 0); |
There was a problem hiding this comment.
❓ @jaasen-livefront, is there a reason we will need the whole cipher object? since we just need the cipher Ids, we can add a .Select(c => c.Id) to this
There was a problem hiding this comment.
@gbubemismith Good point! Fixed 2cac61e
|
3 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-33500
📔 Objective
Fix orphaned attachment files left behind when ciphers are deleted in bulk, an organization vault is purged, or an organization is deleted.
When a single cipher is deleted via
CipherService.DeleteAsync, attachment files are properly removed from blob/disk storage viaDeleteAttachmentsForCipherAsync. However, three other deletion paths only removed DB records, leaving attachment files permanently orphaned in storage:CipherService.DeleteManyAsync(bulk cipher delete) — deleted cipher rows but never cleaned up attachment storage.CipherService.PurgeAsync(org vault purge) — deleted cipher rows but never cleaned up attachment storage.OrganizationDeleteCommand.DeleteAsync(org deletion) — cascade-deleted cipher rows via SQL stored proc but never cleaned up attachment storage.Changes
CipherService.DeleteManyAsync: After DB deletion, iterate deleted ciphers and callDeleteAttachmentsForCipherAsyncfor each.CipherService.PurgeAsync: Fetch org cipher IDs before DB deletion, then clean up each cipher's attachments after.OrganizationDeleteCommand.DeleteAsync: InjectICipherRepositoryandIAttachmentStorageService. Fetch org cipher IDs before DB cascade delete, then clean up each cipher's attachments after.DeleteManyAsync(org admin + personal owner paths) andOrganizationDeleteCommand.Delete_Successto assert attachment cleanup. Added newPurgeAsync_WithOrganizationId_DeletesCiphersAndAttachmentstest.📸 Screenshots